🔴 Triage SkipDeletionOnly silently skips review of security-relevant code removals

The new triage system classifies any diff where all non-context changes are Removed as SkipDeletionOnly, causing these files to be silently skipped from LLM review (src/review/pipeline.rs:311-320). This means removal of security checks, validation logic, error handling, authentication guards, or rate-limiting code would never be reviewed.

- if !user.is_authorized() {
-     return Err("Unauthorized");
- }

Deletion-only changes are a well-known category of security-relevant refactors. Unlike the pre-existing diff.is_deleted check (which skips fully deleted files — a reasonable heuristic), SkipDeletionOnly applies to files that still exist but had code removed, which is exactly the pattern for accidentally removing safeguards.

In src/review/triage.rs, remove the SkipDeletionOnly classification entirely, or make it configurable. The deletion-only check at lines 89-95 in triage_diff() should be removed because deletion-only changes can represent security-relevant code removals (e.g., removing authentication checks, validation, error handling). If kept at all, it should be gated behind a config flag like triage_skip_deletions: bool (default false). Also update the TriageResult enum, should_skip(), and reason() to remove or gate the variant. In src/review/pipeline.rs around line 311-320, the triage skip logic is fine as-is — it just needs the underlying triage heuristic to be less aggressive.

Was this helpful? React with 👍 or 👎 to provide feedback.

🟡 COMMENT_PREFIXES includes "* " which false-positives on Markdown list items and other non-comment content

The is_comment_line function at src/review/triage.rs:176-193 uses COMMENT_PREFIXES to detect comment-only changes, but it's file-type-agnostic. The prefix "* " (intended for C/Java/Rust doc-comment continuation lines like * @param x) also matches Markdown list items (* Fix the build process). A diff that only modifies Markdown list items, horizontal rules (--), or headings (# ) in .md/.sql/other files would be classified as SkipCommentOnly and silently skipped from review. For example, changes to README, CHANGELOG, or API documentation that only modify list items would never be reviewed.

In src/review/triage.rs, the is_comment_line function (line 176) should either: (1) accept the file extension as a parameter so it can apply language-specific comment prefix lists, or (2) the triage_diff function should exclude known non-code file extensions (.md, .txt, .rst, .adoc) from the SkipCommentOnly classification. The COMMENT_PREFIXES list at line 44 is correct for code files but causes false positives for Markdown/documentation files where '* ' is a list item, '# ' is a heading, and '--' is a horizontal rule.

Was this helpful? React with 👍 or 👎 to provide feedback.

🔴 GitHub Actions secret directly interpolated into shell command allows injection

At line 33, ${{ secrets.OPENAI_API_KEY }} is expanded by GitHub Actions' template engine before the shell runs, meaning the raw secret value is pasted directly into the if [ -z "..." ] command. If the secret contains shell-special characters (e.g., ", `, $(...), ;, |), it can break the script or, worse, execute arbitrary commands.

- name: Check API key
  id: check_key
  env:
    KEY: ${{ secrets.OPENAI_API_KEY }}
  run: |
    if [ -z "$KEY" ]; then
      echo "skip=true" >> "$GITHUB_OUTPUT"
    else
      echo "skip=false" >> "$GITHUB_OUTPUT"
    fi

Was this helpful? React with 👍 or 👎 to provide feedback.

🟡 find_enclosing_boundary_line ignores max_search_lines parameter, violating its documented contract

The function's docstring at src/core/function_chunker.rs:285-287 states it "Searches upward from line up to max_search_lines", but the implementation at line 292 prefixes the parameter with _ and completely ignores it. Instead it scans all function boundaries in the entire file. The caller at src/core/context.rs:56-57 passes 10 and then applies its own post-hoc filter (boundary >= start.saturating_sub(10)) to compensate, but if anyone else calls find_enclosing_boundary_line trusting the documented parameter semantics, the limit won't be respected.

In src/core/function_chunker.rs at line 292, the _max_search_lines parameter is unused. Either implement the documented behavior (filter boundaries to only those within max_search_lines above the given line), or remove the parameter from the function signature and update the docstring. If you keep it, the implementation should filter like: find_enclosing_function(&boundaries, line).filter(|b| line - b.start_line <= max_search_lines).map(|b| b.start_line).

Was this helpful? React with 👍 or 👎 to provide feedback.